docs(#181): add shell return-vs-exit correctness rule to AGENTS.md#204
docs(#181): add shell return-vs-exit correctness rule to AGENTS.md#204fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
Add a new "Shell scripting" section to AGENTS.md with guidance on the return-vs-exit distinction in executed vs sourced scripts. The review agent previously dismissed a top-level return statement in an executed script as "harmless dead code" (PR fullsend-ai#3182). In bash, return at script top level is invalid and causes a hard abort under set -euo pipefail, regardless of reachability. This new rule instructs reviewers to flag top-level return (outside a function body) in executed scripts as a medium-severity correctness finding, especially when code is being inlined from a sourced library. Note: pre-commit could not run in-sandbox due to a network error installing shellcheck-py (exit 3). The post-script runs an authoritative pre-commit check on the runner. Closes #181
Review — approvePR #204 adds a "Shell scripting" section to AGENTS.md documenting the SummaryThis is a focused, documentation-only change (10 lines added to AGENTS.md) that adds a new review rule based on a real bug found in PR fullsend-ai#3182. The added content is technically accurate, properly authorized by issue #181, follows existing formatting conventions, and is placed logically in the document structure. No security concerns were identified. FindingsAll findings are low severity and non-blocking. 1. Technical accuracy nuance (low) — 2. Sub-agent awareness (low, downgraded from medium) — 3. Section placement (low) — VerdictApprove. The change is technically sound, properly scoped to issue #181, and follows the document's existing conventions. The low-severity findings are non-blocking observations for potential follow-up.
Previous runReview — PR #204Verdict: comment · 1 medium, 2 low findings · non-blocking OverviewThis PR adds a "Shell scripting" section to AGENTS.md with a The core claims are technically correct: Findings1. Imprecise failure mode description —
|
| Severity | medium |
| Category | technical-accuracy |
| File | AGENTS.md |
The text states: `return` causes a hard error — and under `set -euo pipefail`, this aborts the script.
The phrase "hard error" implies a fatal, unrecoverable condition. In practice:
- Without
set -e: a top-levelreturnproducesbash: return: can only 'return' from a function or sourced scripton stderr and sets exit status 2, but execution continues past thereturn. This is a control-flow bug (unintended fallthrough), not an abort. - With
set -e: the non-zero exit status from the failedreturntriggersset -e, which then aborts the script.
The abort is caused specifically by -e, not by -u or -o pipefail. Attributing the behavior to the full set -euo pipefail triad is slightly misleading. A reviewer encountering a script without set -e might misdiagnose the symptom (continuation vs. abort).
Suggested improvement: Clarify that without set -e, return produces a non-fatal error and execution falls through — a different but equally real bug. Under set -e (typically via set -euo pipefail), the non-zero exit status causes abort.
This doesn't change the review guidance outcome (flag it either way), but precise failure mode description helps with root cause analysis.
2. Edge case: dual-mode scripts not addressed
| Severity | low |
| Category | edge-case-omission |
| File | AGENTS.md |
Some scripts are designed to work both when sourced and when executed, typically using if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then .... In such dual-mode scripts, a top-level return is valid in the sourced code path. A reviewer following this rule literally might flag it incorrectly. This is a minor edge case and may not warrant inclusion, but is worth noting for completeness.
3. Minor style inconsistency in reviewer guidance label
| Severity | low |
| Category | formatting-consistency |
| File | AGENTS.md |
The new section uses **When reviewing shell scripts:** while the existing convention in AGENTS.md is **When reviewing PRs:** (used in Forge abstraction and ADR sections). The deviation is defensible — it scopes the guidance more precisely — but breaks the established formatting pattern.
What looks good
- Authorization: Change traces directly to issue Review agent: add shell correctness rule for top-level
returnin executed scripts #181 with matching scope — no scope creep. - Technical accuracy: Core claims about
returninvalidity, load-guard patterns, and abort behavior underset -euo pipefailare verified correct. - Security: No secrets, injection patterns, Unicode steganography, or permission changes. The review directive follows the existing instructional style in AGENTS.md.
- Documentation currency: No staleness introduced. README.md does not maintain a section index for AGENTS.md.
- Section placement: Creating
## Shell scriptingas a peer to## Go codeis a reasonable structural choice for language-specific guidance.
Previous run (2)
Review — PR #204
Verdict: Approve
Summary
This PR adds a 10-line "Shell scripting" section to AGENTS.md with guidance on the return vs exit distinction in executed vs sourced shell scripts. The change directly implements the specification from issue #181, which documented a missed review finding where a top-level return in an executed script was dismissed as "harmless dead code."
Dimension results
| Dimension | Result |
|---|---|
| Correctness | ✅ Pass (1 low finding) |
| Security | ✅ Pass |
| Intent & coherence | ✅ Pass — change matches issue #181 exactly, scope is tight |
| Style & conventions | ✅ Pass — follows all established AGENTS.md patterns |
| Documentation currency | ✅ Pass |
| Cross-repo contracts | ⏭ Skipped — no exported interfaces modified |
Findings
[low] Technical precision — "hard error" characterization (AGENTS.md)
The phrase "return causes a hard error" is slightly imprecise. Without set -e, a top-level return in an executed script emits an error diagnostic and sets exit status 2, but the script continues executing subsequent commands. The abort only occurs when set -e (errexit) is active, because the non-zero exit status triggers errexit. The current wording could be read as implying return alone terminates the script.
The guidance is pragmatically correct for scripts following the set -euo pipefail convention (which is standard in this project), and the sentence does qualify with "under set -euo pipefail." This is a minor precision improvement, not a blocking issue.
Suggested rewording: "return produces an error (exit status 2) — and under set -e (commonly part of set -euo pipefail), this non-zero status triggers errexit and aborts the script."
Notes
- The change is properly authorized by issue Review agent: add shell correctness rule for top-level
returnin executed scripts #181 and matches the proposed text verbatim. - Placement after the "Running e2e tests" section is reasonable; the issue referenced an existing "Shell scripting" section with
gh api --paginateguidance that doesn't exist in this fork, so creating a new top-level section is the correct adaptation. - The repo has 65+ shell scripts across multiple directories, all using
returncorrectly (only in functions or sourced libraries), confirming the relevance of this guidance.
Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.Protected files in this PR:
AGENTS.md
Labels: PR adds shell scripting documentation to AGENTS.md
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
|
/fs-review |
fullsend-ai-review
Bot
added
requires-manual-review
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 10:19 AM UTC · Completed 10:27 AM UTC |
|
|
||
| ## Shell scripting | ||
|
|
||
| ### `return` vs `exit` in executed scripts |
There was a problem hiding this comment.
[low] technical-accuracy
The statement "return causes a hard error" is slightly imprecise. A top-level return in an executed script produces exit status 2 and an error message, but without set -e the script continues execution past the failed return. The abort only occurs because set -e treats the non-zero exit status as fatal. The practical guidance is correct for scripts using set -euo pipefail, but the mechanism description could mislead a reviewer analyzing a script without set -e.
Suggested fix: Consider rewording to: "return is invalid at the top level of an executed script — bash emits an error and the statement fails with exit status 2. Under set -e (commonly set -euo pipefail), this non-zero status aborts the script."
|
|
||
| If only `E2E_GITHUB_USERNAME` and a password source are available, `make e2e-test` will automatically generate a session file before running tests. See `make help` for all available targets. | ||
|
|
||
| ## Shell scripting |
There was a problem hiding this comment.
[low] structure
The new Shell scripting section sits between the Running e2e tests subsection (under Go code) and the Forge abstraction section. The placement is reasonable but creates a slight topic jump from Go-specific testing to general shell scripting.
fullsend-ai-review
Bot
added
requires-manual-review
1 participant
Add a new "Shell scripting" section to AGENTS.md with guidance on the return-vs-exit distinction in executed vs sourced scripts.
The review agent previously dismissed a top-level return statement in an executed script as "harmless dead code" (PR fullsend-ai#3182). In bash, return at script top level is invalid and causes a hard abort under set -euo pipefail, regardless of reachability. This new rule instructs reviewers to flag top-level return (outside a function body) in executed scripts as a medium-severity correctness finding, especially when code is being inlined from a sourced library.
Note: pre-commit could not run in-sandbox due to a network error installing shellcheck-py (exit 3). The post-script runs an authoritative pre-commit check on the runner.
Closes #181
Post-script verification
agent/181-shell-return-rule)f96750babbed5ada406a9ae04e8068449701d9c7..HEAD)